AsyncRouter#176
Conversation
| AsyncRouter::~AsyncRouter() { | ||
| for (AsyncCallbackWebHandler* handler : _handlers) { | ||
| _server->removeHandler(handler); | ||
| delete handler; |
There was a problem hiding this comment.
What is the handler is a static class instance created by the user ?
mathieucarbou
requested review from
a team,
Copilot,
mathieucarbou,
me-no-dev and
vortigont
There was a problem hiding this comment.
Pull Request Overview
This PR introduces the AsyncRouter feature to modularize route handling in the AsyncWebServer. Key changes include:
- Adding overloaded addRouter methods (for unique_ptr and raw pointer) and corresponding removeRouter functionality.
- Creating a new AsyncRouter class with its header and implementation.
- Providing an example usage in router.ino.
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/WebServer.cpp | Added AsyncRouter add and remove methods to AsyncWebServer. |
| src/ESPAsyncWebServer.h | Declared AsyncRouter methods and included AsyncRouter.h with updated documentation. |
| src/AsyncRouter.h | Introduced the AsyncRouter class interface. |
| src/AsyncRouter.cpp | Implemented the AsyncRouter class including route handling and middleware addition. |
| examples/Router/router.ino | Provided example usage demonstrating how to use the new AsyncRouter functionality. |
Comments suppressed due to low confidence (1)
src/AsyncRouter.cpp:35
- The method name 'addMidleware' appears to contain a typo. Consider renaming it to 'addMiddleware' to improve clarity.
void AsyncRouter::addMidleware(AsyncMiddleware* middleware) {
| * a deep copy of the pointer object will be created, | ||
| * it is up to user to manage further lifetime of the object in argument | ||
| * | ||
| * @param router pointer to router object to copy setting from | ||
| * @return AsyncRouter& reference to a newly created router |
There was a problem hiding this comment.
The documentation for addRouter(unique_ptr) mentions that a deep copy of the router object will be created, but the implementation instead transfers ownership. Please update the comment to accurately reflect the behavior.
| * a deep copy of the pointer object will be created, | |
| * it is up to user to manage further lifetime of the object in argument | |
| * | |
| * @param router pointer to router object to copy setting from | |
| * @return AsyncRouter& reference to a newly created router | |
| * Transfers ownership of the router object to the method. | |
| * The caller is no longer responsible for managing the lifetime of the object. | |
| * | |
| * @param router unique pointer to the router object to be added | |
| * @return AsyncRouter& reference to the added router |
| * this will NOT deallocate pointed object itself, internal router with same path will be removed if any | ||
| * it's a compat method, better use `removeRouter(const char* path)` |
There was a problem hiding this comment.
The comment states that removeRouter(AsyncRouter* router) will NOT deallocate the pointed object, yet the implementation removes the unique_ptr from the container (which will deallocate the object). Clarify the intended ownership and deallocation behavior in the documentation.
| * this will NOT deallocate pointed object itself, internal router with same path will be removed if any | |
| * it's a compat method, better use `removeRouter(const char* path)` | |
| * this will deallocate the pointed object, as the internal container uses unique_ptr for ownership. | |
| * It's a compat method, better use `removeRouter(const char* path)` |
|
This PR is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 7 days. |
|
This PR was closed because it has been stalled for 7 days with no activity. |
No description provided.